[3.15] gh-111487: Fix csv.Sniffer never testing the minimum delimiter consistency - #154872

I've corrected this PR: the earlier description and NEWS entry said it fixed a delimiter "occurring on exactly 90% of the rows", and that was wrong. The score has the non-matching rows subtracted from the mode's count, so 90% of rows scores 80%, and a 90% score needs 95% of rows. The code was right; my description of it was not.

I also quoted @rhettinger's "we could use exact fractional arithmetic" without his following sentence, which recommended against making the accumulation more exact. That was careless of me and gave a misleading impression of support. His full comment is quoted in the updated description, and I've tried to answer it directly.

The scope section is new and is the important part: this does not make the sample from the issue report work, and I've said so explicitly rather than letting the PR imply otherwise.

@serhiy-storchaka you said on the issue that a targeted fix for the maintenance branches would be welcome, so here's one, but it's narrower than your example and I'd rather be upfront about that.

It doesn't make your sample (9 rows with 30 delimiters, one with 29) sniff. I don't think anything targeted can. The old scoring subtracts the non-matching rows from the mode's count, so that sample scores 8/10 = 80%, nowhere near the threshold, with or without the bug fixed here. 3.16 handles it because gh-83273 dropped _guess_delimiter entirely and replaced it with relative scoring, and backporting that to a beta branch seems like the opposite of what anyone wants.

What's left is small but definite. The consistency counter is stepped down by repeated -= 0.01 on a float, so it bottoms out at 0.9099999999999999 and the pass at the documented 90% never runs. Counting down in whole percent with integer arithmetic fixes that, and makes each comparison the exact form of the float one it replaces.

On @rhettinger's point from 2023 about code on the margins breaking: both halves move behaviour towards what's documented, and since the loop stops as soon as delims is non-empty, the extra pass only runs when nothing was found at all. It can add a delimiter that was being missed, it can't drop one that was already picked up.

Happy to do 3.14, and 3.13 if that's still in scope.

Would not reducing threshold from 0.9 to 0.899999 have the same effect?

Yes, it does. I tested it rather than assuming: with threshold = 0.899999 the loop makes an eleventh pass at 0.8999999999999999 and the test in this PR passes, exactly as it does with the integer version. Your sample from the issue still doesn't sniff under either one, which is what I'd expect.

I also checked whether the two can ever disagree, since that was the part I wasn't sure about. The accumulated error only ever makes the float counter smaller than the whole percent it stands for, by about 9e-17 at the last pass, so it can only let a delimiter through, never hold one back. For the two to select a different pass, a score would have to land in a ~1e-16 window below a percent boundary, and as it's a ratio of two row counts that needs something like 1e16 rows. I swept every (total, matching) pair for totals up to 3000 and found no case where they differ.

So it's the same fix in one line, and on a maintenance branch that's a good argument by itself. The only thing I'd put against it is that 0.899999 only reads correctly to someone who already knows about the drift, and tidying it back to 0.9 later would quietly undo it. A comment covers that.

I don't have a strong preference here. Say which you'd rather have and I'll push it.

The simplest way is to add 1e-9 or something like when you compare two floats.

Pushed — it's consistency >= threshold - epsilon with epsilon = 1e-9 now, and the integer arithmetic is gone. It's the better change for this branch, for a reason I hadn't seen until you suggested it.

There are two float comparisons in that loop, and only the guard needs the tolerance. Of the eleven counter values, one is exact (1.0) and the other ten all land below the whole percent they stand for — never above. So (v[1]/total) >= consistency never rejects a score that sits exactly on a documented percent, and it can be left alone.

That's what makes this better than what I had. Replacing the inner comparison too meant "the only change is the extra final pass" was something I could measure but not prove. With the tolerance on the guard alone it's the same values in the same order plus one more at the end, so nothing that matched before can stop matching, by construction. I compared the two anyway over 6795 samples — row and inconsistency sweeps across four delimiters, plain CSV, TestSniffer's own corpus, the sample from the issue, and 6000 random strings — no disagreements, and both differ from unpatched on the same 16, every one of them Could not determine delimiter becoming the right delimiter.

The test is unchanged; the NEWS entry just describes the fix differently.

One thing I'd rather raise than quietly ignore: 3.15.0rc1 was tagged on the 4th, so this is now a behaviour change against a branch in rc. It's small and one-directional, but "the sniffer accepts a delimiter it used to reject" is still a behaviour change, and that call is yours or the release manager's rather than mine. If it's too late for 3.15.0, I'll happily retarget at 3.14 and 3.13 — they're affected identically.

Advice: tell Claude to simplify and shorten any message and comment. Shorter text has larger chance to be read and accepted.